Reuse a single per-file diskcache handle when streaming remote files - #15049
Reuse a single per-file diskcache handle when streaming remote files#15049jamalex wants to merge 2 commits into
Conversation
github-actions
Bot
added
DEV: backend
Measured speedup on a Raspberry Pi (SD-card storage)Reproduced and verified on the affected device. Setup: Raspberry Pi (4 cores, ~8 GB RAM), Kolibri 0.19.5, content storage on the SD card ( Method: fetched fresh/uncached proxied files ( Before vs after (6 concurrent, uncached)
A 185-byte subtitle file took 37 s in an earlier before-run purely from contending with a concurrent download, which was clear evidence this was storage I/O contention, not bandwidth or file size. After the change the whole batch finishes in a couple of seconds, and the remaining time is used by actual network download, not overhead (two after-runs measured 4.1 s and 1.3 s for the video as upstream conditions varied). Why
A 13 MB file triggered ~415 of these opens (about 4 per chunk); this PR reuses one handle per file, so a full stream now opens the cache once regardless of size (verified). On SSD/NVMe the repeated opens are microseconds, which is why it never showed up on desktop installs; it only hits us on slow storage like an SD card. |
e15debd to
4c917ff
Compare
Build Artifacts
Smoke test screenshot |
…te files Streaming a proxied remote file (RemoteFile) reopened the per-file chunk diskcache on essentially every read block, which is cheap on fast storage but pathologically slow on SD cards. Cover: - the diskcache is constructed a bounded number of times regardless of file size (not once per chunk) - concurrent readers of the same file, and single-read correctness - the handle self-heals when the chunk directory is evicted and recreated mid-stream, including dropping a borrowed handle without closing it - the self-heal mechanism and Windows-safety invariant cross-platform: _open_cache reopens on an incarnation change, and delete() releases the handle before removing the directory - the marker-file fallback used when the filesystem reports no inode - eviction skips a directory that cannot be removed rather than aborting The end-to-end deletion flavors are POSIX-only, since Windows forbids removing a directory whose diskcache is held open; tests release the handle before removing a directory so it does not block removal there. Claude-Session: https://claude.ai/code/session_01MnAmUbaCBpL4wBKFYUyCwt
…read block ChunkedFile opened a fresh diskcache.Cache on every cache access, and while streaming a proxied file RemoteFile spawns a FileDownload per chunk, so serving one file constructed the cache O(file size) times. Each construction opens a SQLite connection (pragmas + fsync), negligible on SSD/NVMe but collapsing proxied browsing on SD-card devices: a 13.7MB file took ~8 minutes under concurrency and 34KB thumbnails ~50s; both drop to ~1-4s with this change. Memoize the Cache handle on the ChunkedFile and reuse it for the object's lifetime, and let a RemoteFile share its handle with the FileDownload streaming into the same file, so a full stream constructs the cache once regardless of size. To keep the memoized handle correct when the chunk directory is deleted and recreated (e.g. streamed cache eviction racing a stream), _open_cache stamps each handle with a per-incarnation token - the directory inode, or a marker file on filesystems without a usable inode (FAT/exFAT, some Android) - and reopens when it changes, restoring the self-healing that opening per access gave for free. Because the handle now stays open for the stream's lifetime, release it before removing the directory: delete() closes it first (an open SQLite file blocks removal on Windows), and eviction skips - rather than aborts on - a directory it cannot remove. Also stop resetting the read position in _initialize (now reachable mid-read via ensure_writable, where it would rewind an in-progress read). Claude-Session: https://claude.ai/code/session_01MnAmUbaCBpL4wBKFYUyCwt
4c917ff to
98c42b1
Compare
2 participants
Summary
Addresses per-chunk disk-based lock cache churn when streaming content from another library, by re-using a cache across operations within a RemoteFile/ChunkedFile. This was most impacting devices with slow disk I/O, like an RPi running off of an SD card, rendering peer or Studio library browsing unusably, unreasonably, and unnecessarily slow.
References
Fixes #15048.
Reviewer guidance
kolibri/utils/file_transfer.py:299_open_cache— the memoized handle is stamped with a chunk-dir "incarnation" (_chunk_dir_incarnation, :256) and dropped/reopened when it changes; confirm the token stays stable while chunk files are written into the dir (it uses the inode alone, deliberately not mtime/ctime, which would churn) yet changes on delete+recreate, and that a borrowed handle is dropped without being closed.kolibri/utils/file_transfer.py:331_initializeno longer resetsself.position; confirm no caller other than__init__relied on that reset (it's now reachable mid-read viaensure_writable).kolibri/utils/file_transfer.py:179_do_file_evictionnow skips a directory it cannot remove; confirm skipped directories aren't counted as freed and the pass still continues.AI usage
Used Claude Code to profile the regression on the affected Raspberry Pi, implement the fix, and write the regression tests. Verified with the full
file_transferandchunked_filetest suites, a before/after benchmark on the device, and an adversarial local code review. (I wrote the Summary myself manually using my fingers, as per decree.)